Use repl language tag for sample #1107
Conversation
| W = @node selectrows(Xs, @node shuffle(rs)) | ||
| julia> Xs = source(X) | ||
| julia> rs = @node rows(Xs) | ||
| julia> W = @node selectrows(Xs, @node shuffle(rs)) | ||
|
|
There was a problem hiding this comment.
Generally, the pattern has been to only include julia> before input when corresponding output is to follow. Are you finding this confusing?
There was a problem hiding this comment.
Yes, quite a bit. There are two reasons for this, one is that prompted and prompt-less code are mixed in, and it's not always (immediately) clear to tell what's code, what's code before the output, and what's output. This is especially highlighted when object's reprs are shown, which are made to look like Julia constructors/code, and so it gets even harder to tell what's output and what's code. The other part is syntax highlighting, which can be a huge boon when reading online sources quickly for some reference.
There was a problem hiding this comment.
Good to have this feedback, thank you.
Unfortunately, this convention is pretty widespread, at least in the MLJ ecosystem. I wonder if others are also finding this confusing. @OkonSamuel @EssamWisam What's your view of this? Do we need to include julia> prompts in every single line when there is sometimes REPL output to be included in a docstring? Often we just only add julia> before lines with output immediately following.
There was a problem hiding this comment.
My preference is to minimize the use of >julia as it jams up the code (and has to be dropped before copy-pasting into .jl files) which as you said is consistent with the ongoing convention for MLJ. That said, I do find @abhro's point to be valid; it would be not immediately obvious to a beginner that the printed object is not an actual function being called and for that I think we could expose to readers the convention that what immediately comes after a line with >julia is the output of the code in that line.
In any case, in the future, we can explore the possibility of having the outputs be printed in a differently styled cell as in Imbalance tutorials and DataScienceTutorials which would allow dropping all >julia and would be optimal for maximal facilitation of code reuse.
There was a problem hiding this comment.
Yes, I think the way the @example macro does it also provides a good separation between output and code.
docs/src/transformers.md
Outdated
| # transforming: | ||
| Xsmall = transform(mach); | ||
| selectrows(Xsmall, 1:4) |> pretty | ||
| julia> # transforming: |
There was a problem hiding this comment.
This is really weird: a comment at the julia> prompt, here and later. Let's just skip all the prompts that don't precede a block of output (see previous comment). Here and below in this file.
There was a problem hiding this comment.
Yes, I'm not particularly happy with this change either, I just did it for consistency. Maybe there's a better way to write this?
There was a problem hiding this comment.
To be clear, I meant the prompted lines that only hold comments. I still hold that for any code sample that mixes code and output, the code should be prompted.
There was a problem hiding this comment.
Maybe a better way to do this would be to split the code sample into 3, one for setup, one for transforming, and one for predicting?
There was a problem hiding this comment.
Yes, you can re-organize so that the code comments become ordinary markdown text between seperate fenced blocks.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1107 +/- ##
=======================================
Coverage 57.89% 57.89%
=======================================
Files 2 2
Lines 38 38
=======================================
Hits 22 22
Misses 16 16 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Anthony Blaom, PhD <anthony.blaom@gmail.com>
Remove julia> prompts, replace with @example macro
|
@abhro My question regarding |
|
Sure! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1107 +/- ##
=======================================
Coverage 57.89% 57.89%
=======================================
Files 2 2
Lines 38 38
=======================================
Hits 22 22
Misses 16 16 ☔ View full report in Codecov by Sentry. |
|
@EssamWisam Can you please go over the proposed changes to the cheatsheet. @abhro Can you please address the ParallelKMeans comment? |
|
Hello, I think I addressed it in the original comment thread? But to reiterate, this is to support the change in docs/src/transformers.md, where I changed the predicting transformers code samples to an |
There was a problem hiding this comment.
Reviewed the cheatsheet. All is okay. However, I wonder why only some of the backticks were replaced with ```julia ``` and not others. For the purposes of viewing the cheatsheet in the website, it's better to use ```julia ``` whenever the line is long enough.
Otherwise, the cheatsheet, viewed from the website, would look like this:
where many lines are not syntax highlighted (e.g., look below Learning Curves).
Other great changes to facilitate viewing the cheatsheet from the browser:
- No overly long sections (chunk tuning into three ## headlines: ## Tuning model wrapper, ## Ranges for tuning (
range=...), ## Tuning strategies) - No overly long lines of code. For instance, the following is too long to view on the website. E.g., comments in:
@abhro let me know if you would be willing to revisit the cheat sheet accordingly. If not, I can consider that and add a commit.
For how the sheet views on the website after doing the change in the first bullet above:
Thank you for the effort.
Thanks for the explanation. ParallelKMeans is not a regularly maintained package. Can we please change |
|
Hello, @EssamWisam, thank you for the feedback! I'm working on updating the cheatsheet to make it a little more consistent. Can I ask how you generated the pdf? It would help a lot in checking the changes I'm making. Thanks again! |
|
@ablaom I can't find the KMeans model in NearestNeighborsModel. I could use the one in Clustering/MLJClusteringInterface. Does that work? |
Oops 😳 Yes, use the MLJClusteringInterface version. |
Well, the MLJ website is currently under development and (as of now only) unless you have some web development frameworks expertise, it's not straightforward to generate it. That said, avoiding overly ong long lines, using ```julia ``` more often and avoiding overlong long sections are pretty much all the conditions needed for the cheatsheet to render nicely. |
|
Alrighty! I think I've made those changes. If there's something I missed, or in some ways it could be made better, please don't hesitate to let me know! Thanks! |
Thank you for the valuable contribution, @abhro. I made some minor comments. |
Co-authored-by: Essam <essamwisam@outlook.com>
No description provided.